-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: PC-13893 Deprecate usage of objective's value field for Composite SLOs #295
Conversation
# Conflicts: # go.sum
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm no terraform expert but I tested the code running it locally and its logic works as it should:
- when
value = 0
composite slo is created - when
value!=0
there's an error - when
value
doesn't exist composite slo is created
but I noticed that when you don't pass value
terraform apply shows:
(⎈|arn:aws:eks:eu-central-1:922330643383:cluster/dev-demo-4:flink)➜ comp-val-test git:(PC-13893-composite-value) ✗ terraform apply
nobl9_project.test: Refreshing state... [id=pkw-test-tf]
nobl9_service.test: Refreshing state... [id=pkw]
nobl9_slo.composite_slo: Refreshing state... [id=pkw-test-tf-composite]
Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
~ update in-place
Terraform will perform the following actions:
# nobl9_slo.composite_slo will be updated in-place
~ resource "nobl9_slo" "composite_slo" {
id = "pkw-test-tf-composite"
name = "pkw-test-tf-composite"
# (6 unchanged attributes hidden)
- objective {
- display_name = "OK" -> null
- name = "tf-objective-1" -> null
- primary = false -> null
- target = 0.8 -> null
- time_slice_target = 0 -> null
- value = 0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000005 -> null
# (1 unchanged attribute hidden)
- composite {
- max_delay = "45m" -> null
- components {
- objectives {
}
}
}
}
+ objective {
+ display_name = "OK"
+ name = "tf-objective-1"
+ target = 0.8
+ value = 0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000005
# (1 unchanged attribute hidden)
+ composite {
+ max_delay = "45m"
+ components {}
}
}
# (1 unchanged block hidden)
}
Plan: 0 to add, 1 to change, 0 to destroy.
╷
│ Warning: SLO objective unique identifier warning
│
│ with nobl9_slo.composite_slo,
│ on entire-budget.test.tf line 29, in resource "nobl9_slo" "composite_slo":
│ 29: resource "nobl9_slo" "composite_slo" {
│
│ Nobl9 is introducing an SLO objective unique identifier to support the same value for different SLIs in the same SLO. As such, Nobl9 is adding a name identifier to
│ each SLO objective. Objective names can be set now, and they'll be required once grace period ends. For more detailed information, refer to:
│ https://docs.nobl9.com/features/slo-objective-unique-identifier
╵
Do you want to perform these actions?
Terraform will perform the actions described above.
Only 'yes' will be accepted to approve.
Enter a value:
Notice how value
is a very long string when I didn't even specified it in my terraform resource I tried to apply:
terraform {
required_providers {
nobl9 = {
source = "nobl9.com/nobl9/nobl9"
version = "0.29.2"
}
}
}
provider "nobl9" {
organization = "nobl9-dev"
project = "default"
ingest_url = "https://dev-demo-1.nobl9.dev/api"
okta_org_url = "https://accounts.nobl9.dev"
okta_auth_server = ""
client_id = ""
client_secret = ""
}
resource "nobl9_project" "test" {
name = "pkw-test-tf"
}
resource "nobl9_service" "test" {
name = "pkw"
project = "pkw-test-tf"
}
resource "nobl9_slo" "composite_slo" {
name = "${nobl9_project.test.name}-composite"
service = nobl9_service.test.name
budgeting_method = "Occurrences"
project = nobl9_project.test.name
time_window {
unit = "Day"
count = 3
is_rolling = true
}
objective {
display_name = "OK"
name = "tf-objective-1"
target = 0.8
composite {
max_delay = "45m"
components {
}
}
}
}
I think this value can be a little confusing to users, perhaps there is a way to avoid displaying it?
I removed the sentinel altogether and now I'm just setting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
## Motivation `value` field of `objective` for Composite SLO is being [deprecated](#295). Before the update in can be made, a N9 platform first needs to be [updated](nobl9/n9#15406) and new version of SDK needs to be [released](nobl9/nobl9-go#549). To be able to release changes in the platform, e2e tests of the latest released SDK and Terraform Provider need to pass against it. ## Summary E2E test assertions will pass with and without [changes](nobl9/n9#15406) in N9 platform. ## Related changes nobl9/nobl9-go#551 nobl9/nobl9-go#549 nobl9/n9#15406 #312 #295 ## Testing Run `make test/e2e` on both environments containing and not containing nobl9/n9#15406.
## Motivation `value` field of `objective` for Composite SLO is being [deprecated](#549). Before the update in SDK can be made, a N9 platform first needs to be [updated](nobl9/n9#15406). To be able to release changes in the platform, e2e tests of the latest released SDK need to pass against it. ## Summary E2E test assertions will pass with and without [changes](nobl9/n9#15406) in N9 platform. ## Related changes #551 #549 nobl9/n9#15406 nobl9/terraform-provider-nobl9#312 nobl9/terraform-provider-nobl9#295 ## Testing Run `make test/e2e` on both environments containing and not containing nobl9/n9#15406.
# Conflicts: # docs/resources/slo.md # go.mod # go.sum # nobl9/resource_slo.go # nobl9/resource_slo_test.go
…te SLOs (#549) ## Motivation Currently, we have `value` field required for all objectives in an SLO. We currently allow to not pass that field in YAML at all but it causes it to default to 0 anyway and is returned `value: 0`. This is inconsistent because GET yaml is different than APPLY YAML in such case. Composite SLOs always have exactly one objective, so as such `value` doesn’t matter for them at all as long as it doesn't change. Moreover, the existence of value is documented and used in examples which is confusing to new adopters of Composite SLOs because it is required, changing it will restart budget, but it doesn’t do anything. We want to encourage and allow not setting `value` field for Composite SLOs while maintaining backward compatibility with users who perhaps already explicitly set it. ## Summary * `value` will be omitted if it is `null` in API. * `value` field is omitted in all Composite SLO examples and E2E tests. ## Related changes #551 #549 nobl9/n9#15406 nobl9/terraform-provider-nobl9#312 nobl9/terraform-provider-nobl9#295 ## Testing ```go package main import ( "context" "github.com/nobl9/nobl9-go/manifest" "github.com/nobl9/nobl9-go/manifest/v1alpha/service" "github.com/nobl9/nobl9-go/manifest/v1alpha/slo" "github.com/nobl9/nobl9-go/manifest/v1alpha/twindow" "github.com/nobl9/nobl9-go/sdk" v1 "github.com/nobl9/nobl9-go/sdk/endpoints/objects/v1" "log" "time" ) func main() { ctx := context.Background() client, err := sdk.DefaultClient() if err != nil { log.Fatalf("failed to create sdk client, err: %v", err) } const project = "value-test" svc := service.New(service.Metadata{Name: "my-service", Project: project}, service.Spec{}) composite := slo.New(slo.Metadata{Name: "my-slo", Project: project}, slo.Spec{ BudgetingMethod: slo.BudgetingMethodOccurrences.String(), Objectives: []slo.Objective{ { ObjectiveBase: slo.ObjectiveBase{ Name: "objective-1", // Look MA! No value! }, BudgetTarget: ptr(0.99), Composite: &slo.CompositeSpec{ MaxDelay: time.Hour.String(), Components: slo.Components{ Objectives: []slo.CompositeObjective{}, }, }, }, }, Service: svc.Metadata.Name, TimeWindows: []slo.TimeWindow{ { Unit: twindow.Day.String(), Count: 1, IsRolling: true, }, }, }) objects := []manifest.Object{svc, composite} if err := manifest.Validate(objects); err != nil { log.Fatalf("failed to validate manifest, err: %v", err) } if err := client.Objects().V1().Apply(ctx, objects); err != nil { log.Fatalf("failed to apply objects, err: %v", err) } slos, err := client.Objects().V1().GetV1alphaSLOs(ctx, v1.GetSLOsRequest{ Names: []string{composite.GetName()}, Project: project, }) if err != nil { log.Fatalf("failed to get slo, err: %v", err) } if slos[0].Spec.Objectives[0].Value != nil { log.Fatalf("expected nil, got %v", *slos[0].Spec.Objectives[0].Value) } log.Println("Composite SLO's value is nil") } func ptr[T any](t T) *T { return &t } ``` ## Release Notes Usage of `spec.objective[0].value` field for Composite SLOs becomes deprecated. * New Composite SLOs should not set `value` field. * New Composite SLOs will still accept `value: 0` for backward compatibility with older versions of Nobl9 SDK and Nobl9 Terrafrom Provider. * If `value` was previously set to `0` for Composite SLO then it should be omitted going forward. * If `value` was previously set in Composite SLO to a number other than `0` then it can no longer be updated but still will be accepted for backward compatibility. The usage of `value` for SLOs using ratio or threshold SLIs does not change.
Motivation
Currently, we have
value
field required for all objectives in an SLO. We currently allow to not pass that field in YAML at all but it causes it to default to 0 anyway and is returnedvalue: 0
. This is inconsistent because GET yaml is different than APPLY YAML in such case.Composite SLOs always have exactly one objective, so as such
value
doesn’t matter for them at all as long as it doesn't change.Moreover, the existence of value is documented and used in examples which is confusing to new adopters of Composite SLOs because it is required, changing it will restart budget, but it doesn’t do anything.
We want to encourage and allow not setting
value
field for Composite SLOs while maintaining backward compatibility with users who perhaps already explicitly set it.Summary
value
field of SLO'sspec.objective
is now optional.value
is removed from all examples of Composite SLOs.Nobl9 SDK is temporarily redirected unreleased version containing unreleased changes required for this to work.
Related Changes
nobl9/nobl9-go#551
nobl9/nobl9-go#549
https://github.com/nobl9/n9/pull/15406
#312
#295
Testing
Release Notes
Usage of
value
field in schema forobjective
(part of SLO resource) becomes deprecated for objectives usingcomposite
section.value
field.value: 0
for backward compatibility with older versions of Nobl9 SDK and Nobl9 Terrafrom Provider.value
was previously set to0
for Composite SLO then it should be omitted going forward.value
was previously set in Composite SLO to a number other than0
then it can no longer be updated but still will be accepted for backward compatibility.The usage of
value
for SLOs using ratio or threshold SLIs does not change.